perf: Optimize round scalar performance #19831
Conversation
| return Ok(ColumnarValue::Scalar(ScalarValue::Float64(None))); | ||
| } | ||
| _ => { | ||
| // Fall through to array path for non-Int32 decimal places |
There was a problem hiding this comment.
Is this arm possible in normal execution?
There was a problem hiding this comment.
No, it's unreachable because decimal_places is coerced to Int32 by the signature.
|
|
||
| match value_scalar { | ||
| ScalarValue::Float64(Some(v)) => { | ||
| let factor = 10_f64.powi(dp); |
There was a problem hiding this comment.
I feel we should use round_float here for consistency?
| ScalarValue::Float32(None) => { | ||
| return Ok(ColumnarValue::Scalar(ScalarValue::Float32(None))); | ||
| } | ||
| // For decimals and other types: fall through to array path |
There was a problem hiding this comment.
Is there anything stopping us from supporting decimals as well?
There was a problem hiding this comment.
The round_decimal function requires the scale parameter from the decimal type (e.g., Decimal128(precision, scale)), which makes extracting and processing scalars more involved than for floats.
For float scalars, we just call round_float(*v, dp) directly. For decimals, we'd need to:
- Extract the native value from ScalarValue::Decimal128 (or other decimal variants)
- Get the scale from the type
- Call round_decimal(value, scale, dp)
- Reconstruct the ScalarValue with precision/scale
If you're comfortable with these changes in this PR for the decimal then I can introduce these as well. What do you think?
2 participants
Which issue does this PR close?
Rationale for this change
The round function currently converts scalar inputs to arrays before processing, even when both value and decimal_places are scalar values. This adds unnecessary overhead for constant folding scenarios like
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
No